Skip to content

Conversation

@timfish
Copy link
Member

@timfish timfish commented Jul 1, 2025

Copy link
Contributor

@jsumners-nr jsumners-nr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding tests. Very disappointing the original author doesn't see the value. I opened #15 that cherry-picks the original commit from upstream. I think we'll have an easier time if we merge that first and then rebase this PR on top of it.

@jsumners-nr
Copy link
Contributor

Ugh. My PR isn't going to work because it needs tests. Can you back out our change to the same file, and:

git remote add datadog [email protected]:DataDog/orchestrion-js.git
git cherry-pick datadog/main 1794b4a81bdc851e1730cf667663f24cf14146a4

on your branch.

@timfish timfish force-pushed the fix/no-clashing-locals branch from b8182ca to 122e4de Compare July 1, 2025 12:08
@timfish
Copy link
Member Author

timfish commented Jul 1, 2025

Very disappointing the original author doesn't see the value

We added code snapshot tests for the JavaScript API but the upstream repo doesn't have a JavaScript API or any snapshot tests. I consider the snapshot tests a bit annoying in some ways because they don't actually test the functionality.

@timfish timfish requested a review from jsumners-nr July 1, 2025 12:16
@timfish timfish merged commit 5f75963 into apm-js-collab:main Jul 2, 2025
1 check passed
@timfish timfish deleted the fix/no-clashing-locals branch July 2, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants